Conversation
Dexus
force-pushed
the
refactor-rewrite-geometryutil.js-as-typescript
branch
from
938990c to
aa18cbe
Compare
willmurnane
left a comment
willmurnane
left a comment
There was a problem hiding this comment.
Most of these comments can be resolved later, but just commenting while I'm looking at this.
|
|
||
| // Build the UMD modules using Rollup | ||
| console.log('Building UMD modules with Rollup...'); | ||
| execSync('npx rollup -c rollup.config.mjs', { stdio: 'inherit' }); |
There was a problem hiding this comment.
I'm not familiar with UMD modules, what benefit do they give?
There was a problem hiding this comment.
|
|
||
| import { NfpCache } from '../build/nfpDb.js'; | ||
| import { HullPolygon } from '../build/util/HullPolygon.js'; | ||
| import * as GeometryUtil from '../build/util/geometryutil.js'; |
There was a problem hiding this comment.
Are both built versions necessary, or should this import the UMD as well?
There was a problem hiding this comment.
both are needed the UMD is for the worker, and the non UMD is used in non worker context.
main/util/geometryutil.ts
Outdated
| x?: number; | ||
| y?: number; | ||
| width?: number; | ||
| height?: number; |
There was a problem hiding this comment.
What about making a bounds?: Bounds to make it clearer what the meaning of these values are?
| height: number; | ||
| } | ||
|
|
||
| export interface QuadraticBezierSegmentDef { |
There was a problem hiding this comment.
Should these be exported? They're not apparently used outside of this file. We should define constructors for the QuadraticBezier class for example, and use those.
| export function almostEqual(a: number, b: number, tolerance?: number): boolean { | ||
| return _internalAlmostEqual(a, b, tolerance); | ||
| } |
There was a problem hiding this comment.
I don't know of a reason to define a private function with a body in it, and then make the public method call that. Why not just put the body here, and get rid of the private function?
main/util/geometryutil.ts
Outdated
| ): boolean { | ||
| const dx = p1.x - p2.x; | ||
| const dy = p1.y - p2.y; | ||
| return dx * dx + dy * dy < distance * distance; |
There was a problem hiding this comment.
| return dx * dx + dy * dy < distance * distance; | |
| return Math.hypot(dx, dy) < distance; |
main/util/geometryutil.ts
Outdated
| return _internalLineIntersect(A, B, E, F, infinite); | ||
| } | ||
|
|
||
| export const QuadraticBezier = { |
There was a problem hiding this comment.
Should this be a class?
main/util/geometryutil.ts
Outdated
| const p1c1: Point = new Point( | ||
| t_p1.x + (t_c1.x - t_p1.x) * t, | ||
| t_p1.y + (t_c1.y - t_p1.y) * t, | ||
| ); |
There was a problem hiding this comment.
Should define and use a lerp function that encapsulates the "partway from one point to another" behavior.
| }, | ||
| }; | ||
|
|
||
| export function getPolygonBounds(polygon: Polygon): Bounds | null { |
There was a problem hiding this comment.
This should be a member function of the Polygon class.
- Added unit tests for all functions in geometryutil.ts - Refactored code for better readability and maintainability - Removed unused imports and variables - Improved type definitions and interfaces polygon needs to updated where it called getPolygonBounds
|
ATTENTION: getPolygonBounds calls against GeometryUtil needs to updated the polygon calls for x,y, width and height |
2 participants
This refactoring needs #131 merged before